Skip to content

feat: add OpenShell-compatible diagnostics - #747

Closed
RamonArjona4 wants to merge 8 commits into
mainfrom
user/ramonarjona4/openShell-logging
Closed

feat: add OpenShell-compatible diagnostics#747
RamonArjona4 wants to merge 8 commits into
mainfrom
user/ramonarjona4/openShell-logging

Conversation

@RamonArjona4

@RamonArjona4 RamonArjona4 commented Aug 4, 2026

Copy link
Copy Markdown
Member

Description

Adds OpenShell-compatible local structured diagnostics and audit records for MXC. Records cover policy identity, sandbox identity, enforcement degradation, network policy, process outcomes, teardown, timeout/kill failures, and configuration rejection while remaining local-only and excluded from SDK stdout/captured output.

The change also hardens diagnostic IPC and redaction, including per-session pipe tokens, user-scoped pipe access, control-character sanitization, and removal of isolation-session credentials from diagnostic output.

Intentional behavior changes:

  • IsolationSession forced termination waits up to five seconds.
  • Diagnostic pipe access is user-scoped rather than unrestricted.
  • File audit records use the documented timestamp-prefixed format.

Documentation was kept at the public product/technology level; newly added documentation does not expose targeted Windows-internal implementation names or private telemetry values.

References

No issue linked.

Validation

  • cargo fmt --all -- --check
  • cargo test --workspace
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo test -p wxc_e2e_tests
  • cargo test -p wxc_e2e_tests -- --ignored (13 Windows E2E tests passed)
  • Windows debug Rust build completed successfully.
  • Security, adversarial, functionality, cross-platform, documentation, test-coverage, and regression reviews completed.

Known environment limitations:

  • The build.bat --debug SDK packaging phase could not resolve ip-address@10.4.0 from the configured npm feed and lacked tsc; no SDK files were changed.
  • One standalone basic-process script encountered 0xC0000135, while the native process-container E2E tests passed.
  • src/Cargo.lock adds sha2; the dependency-feed check should be run by CI/ADO before merge.

Checklist

Issue Type

  • Bug fix
  • Feature
  • Task
Microsoft Reviewers: Open in CodeFlow

Add local structured diagnostics and audit records for sandbox policy, identity, lifecycle, enforcement, network, timeout, and rejection events. Keep diagnostics local and out of SDK output, with redaction and hardened diagnostic IPC.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 7effbd2a-1c76-4c8b-bf1b-a33701a0dc1c
Copilot AI balanced review requested due to automatic review settings August 4, 2026 23:58
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

This comment was marked as outdated.

Harden audit identity handling, report actual network and teardown outcomes, preserve diagnostic sinks across state-aware execution, and correct lifecycle, hashing, identity, and Unicode behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 7effbd2a-1c76-4c8b-bf1b-a33701a0dc1c
Copilot AI review requested due to automatic review settings August 5, 2026 01:04

This comment was marked as outdated.

…and audit log writes

- sanitize_identity now allows through only the closed set of shapes MXC
  itself mints (literal "CLI", sandbox-<16 hex>, iso:/wsb: state-aware ids),
  redacting every other caller-supplied containerId unconditionally. Character/
  length checks alone could not prove a value was opaque vs. caller-chosen
  (e.g. alice, ticket-1234), so the permissive fallback branch is removed.
- write_timestamped_file now assembles each timestamped line (including its
  terminator) into a single buffer and issues one write_all call, preventing
  concurrent writers from interleaving and corrupting the one-JSON-object-
  per-line audit log format.
- Updated audit.rs and policy_identity.rs tests for the stricter redaction
  contract, and updated docs/telemetry/telemetry.md's content-rules bullet
  to describe the closed-set behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 5, 2026 05:35

This comment was marked as outdated.

The 'SECTION: JSON Config' diagnostic block wrote the caller-supplied
config verbatim before the runner validated/rejected it. A one-shot
IsolationSession request's experimental.isolationSession.user bundle
(upn, wamToken) is only rejected by the runner after this point, so
credentials could reach diagnostic sinks in the clear even though the
parsed 'Full ExecutionRequest configuration (redacted)' section below
it already redacted them.

Add wxc_common::diagnostic::redact_raw_config_json, which parses the
raw text and recursively blanks any JSON object key matching the same
secret-bearing markers already used for config-parse-error redaction
(config_deserialize::is_secret_path_field, now shared pub(crate)).
Malformed JSON that cannot be parsed is replaced with a placeholder
rather than emitted raw, since we cannot prove it is credential-free.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 5, 2026 19:00
@RamonArjona4
RamonArjona4 marked this pull request as ready for review August 5, 2026 19:03
@RamonArjona4
RamonArjona4 requested a review from a team August 5, 2026 19:03
@RamonArjona4
RamonArjona4 requested a review from a team as a code owner August 5, 2026 19:03

This comment was marked as outdated.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 7, 2026 19:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.

Suppressed comments (7)

src/backends/appcontainer/common/src/appcontainer_runner.rs:1307

  • firewall_apply_ok() still does not represent all rule-installation outcomes: process_host_list silently continues when DNS resolution or a host-specific create_rule fails. A default-allow policy whose block rule fails therefore records status=success and firewall_applied=true even though the requested restriction was omitted. Track partial host-rule failures and make this aggregate status false (while also cleaning up any rules already installed).
            let plan = NetworkManager::describe_policy(&request.policy);
            let firewall_ok = !plan.rules_will_be_installed
                || matches!(network_manager.firewall_apply_ok(), Some(true));

src/core/wxc_common/src/policy_identity.rs:270

  • Dropping original_url wholesale also drops the proxy scheme. Thus http://proxy:8080 and https://proxy:8080 produce the same policy hash even though clients connect to the proxy differently. Parse and hash the non-secret scheme while continuing to exclude userinfo; otherwise the hash is not sensitive to every enforcement-relevant proxy setting.
    match &proxy.address {
        Some(addr) => {
            out.insert("address".into(), Value::String(addr.address.clone()));
            out.insert("port".into(), Value::Number(addr.port.into()));
        }

src/core/wxc/src/main.rs:1163

  • ParseError::Decode also represents a missing/unreadable config file and valid JSON that fails the discriminator's root-object check, not just malformed JSON. Those cases are now recorded as malformed_json, making the structured rejection reason factually wrong. Preserve separate input-I/O, JSON syntax, and typed-data categories instead of assigning one reason to the whole variant.
                RejectionReason::MalformedJson,

src/core/wxc_common/src/audit.rs:415

  • This shape check cannot establish that an identity was MXC-minted. ProcessContainer passes caller-controlled containerId directly to this function, so a value such as sandbox-deadbeefdeadbeef (or an accepted iso: value) bypasses redaction despite the documented rule that caller identifiers are never logged verbatim. Pass provenance into the API or use a separate always-redacting path for ProcessContainer caller IDs.
    if let Some(hex) = identity.strip_prefix("sandbox-") {

docs/telemetry/telemetry.md:452

  • The implementation now does inherit the driver sink and passes that logger to create_process, so state-aware IsolationSession exec emits these process-boundary records. This paragraph, the table above, and the M-ETW-1 row below still claim the opposite; update all three so operators do not treat present records as unsupported or missing records as expected.
The isolation-session `ProcessTimedOut` and `ProcessKillFailed` records are
emitted by the one-shot runner, where `wxc-exec` supplies the local diagnostic
logger to the backend. The state-aware backend trait does not currently carry a
logger into its `exec` method, so state-aware isolation-session exec remains
unrecorded by these MXC-local process-boundary events.

src/core/wxc/src/main.rs:522

  • State-aware semantic validation happens inside run_state_aware (for example, IsolationSession rejects an invalid user.upn or post-provision filesystem policy there). These failures bypass every earlier log_config_rejected call, and this outcome path emits only a prose diagnostic, so the advertised structured configuration-rejection record is absent. Emit ConfigRejected here for the validation/shape/unsupported error codes while leaving runtime backend failures alone.

This issue also appears on line 1163 of the same file.

    logger.install_thread_diagnostic_sink();
    let mut outcome = mxc_engine::run_state_aware(parsed, dry_run);
    Logger::clear_thread_diagnostic_sink();

src/core/wxc_common/src/policy_identity.rs:137

  • Including schemaVersion makes two requests with identical normalized enforcement fields hash differently solely because they used different schema revisions. That contradicts this module's stated “effective enforcement policy” identity and is especially redundant because config_schema_version is emitted as its own audit field. Exclude schema version from the digest and add a regression test across equivalent versions.

This issue also appears on line 266 of the same file.

    root.insert(
        "schemaVersion".into(),
        Value::String(schema_version.clone()),
    );

Comment thread src/core/wxc_common/src/diagnostic.rs
Comment thread src/backends/appcontainer/common/src/appcontainer_runner.rs
Comment thread src/backends/appcontainer/common/src/base_container_runner.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.

Suppressed comments (8)

src/core/wxc_common/src/audit.rs:436

  • This sanitizer cannot prove an identity was MXC-minted. A ProcessContainer caller can set containerId to values such as iso:alice_ticket, wsb:ticket-1234, or sandbox-deadbeefdeadbeef; each matches an accepted shape and is logged verbatim by the AppContainer audit call sites, despite the stated rule that caller-supplied config values are always redacted. Carry provenance in the API (or use a ProcessContainer-specific sanitizer that always redacts caller IDs) rather than inferring provenance from the string shape.
    let mxc_opaque = identity
        .split_once(':')
        .map(|(prefix, token)| {
            matches!(prefix, "iso" | "wsb")
                && !token.is_empty()

src/core/wxc/src/main.rs:512

  • The state-aware policy hash omits the per-phase backend configuration. That data remains in parsed.experimental_raw and is only deserialized inside dispatch, while parsed.request.experimental contains only telemetry. Consequently, two IsolationSession start requests with different configurationId values produce the same mxc.PolicyHash even though they select different session configurations. Include a secret-redacted canonical projection of the current phase config in the state-aware hash.
    // State-aware dispatch bypasses the one-shot runner funnel, so anchor the
    // effective lifecycle policy here before the request is consumed.
    mxc_engine::log_policy_hash(&parsed.request, logger);

src/core/wxc/src/main.rs:522

  • Configuration errors raised during state-aware dispatch never produce mxc.ConfigRejected. Typed phase-config deserialization and backend validation happen inside run_state_aware, but after this call an Err is only sent to log_state_aware_dispatch_error; the structured rejection helper is not called. Emit ConfigRejected for the validation/unsupported error codes returned here, while keeping runtime lifecycle failures separate.
    logger.install_thread_diagnostic_sink();
    let mut outcome = mxc_engine::run_state_aware(parsed, dry_run);
    Logger::clear_thread_diagnostic_sink();

docs/telemetry/telemetry.md:452

  • This is now outdated: state-aware IsolationSession exec inherits the driver’s diagnostic sinks and passes that logger to create_process, so it does emit ProcessExited, ProcessTimedOut, and ProcessKillFailed. Update this paragraph, the platform-scope row above, and the M-ETW-1 row below so the documented coverage matches the implementation.
The isolation-session `ProcessTimedOut` and `ProcessKillFailed` records are
emitted by the one-shot runner, where `wxc-exec` supplies the local diagnostic
logger to the backend. The state-aware backend trait does not currently carry a
logger into its `exec` method, so state-aware isolation-session exec remains
unrecorded by these MXC-local process-boundary events.

src/core/wxc_common/src/audit.rs:45

  • Remove the accidental embedded rustdoc marker in this sentence.
/// Closed set of audit record names. The `mxc.` prefix namespaces the record/// against unrelated lines sharing the same sink.

src/backends/appcontainer/common/src/appcontainer_runner.rs:1725

  • wait() already calls kill() and then sets teardown_done, but Drop calls kill() again. Since TerminateJobObject commonly returns ERROR_ACCESS_DENIED for an already-terminated job, a normally completed handle can emit a spurious mxc.ProcessKillFailed during drop. Make termination idempotent by skipping it after a successful kill or completed teardown.
        if let Err(error) = self.job.terminate(u32::MAX) {
            if !self.audit_enabled() {
                return Ok(());
            }
            let record = self

src/backends/appcontainer/common/src/base_container_runner.rs:2401

  • The same process tree is terminated once in wait() and again in Drop. The second call can fail solely because the job/process is already terminated, causing a false mxc.ProcessKillFailed on a healthy run. Skip terminate_and_reap after run_teardown has cached its result.
        if let Err((method, error)) = outcome {
            if !self.audit_enabled() {
                return Ok(());
            }
            let record = self

src/core/wxc/src/main.rs:1161

  • ParseError::Decode does not mean the payload is malformed JSON. It also covers valid JSON that cannot deserialize into RequestDiscriminator, such as {"phase":42} or a top-level array. Those requests are currently recorded as malformed_json instead of schema_violation, making the machine-readable rejection reason inaccurate. Preserve serde's syntax/data category through ParseError and map only Syntax/Eof to MalformedJson.
        Err(ParseError::Decode(_)) => {
            // The payload could not even be decoded into JSON, so no backend or
            // field path is known — the record still exists so a rejected run
            // is never invisible.
            log_config_rejected(

Comment on lines +239 to +242
if plan.rules_will_be_installed {
logger.log_line("Applying network firewall rules...");
}
(plan.default_policy, plan.firewall_mode_selected)
@RamonArjona4 RamonArjona4 self-assigned this Aug 8, 2026
@RamonArjona4
RamonArjona4 marked this pull request as draft August 8, 2026 01:03
Co-authored-by: RamonArjona4 <25335379+RamonArjona4@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.

Suppressed comments (5)

src/core/wxc/src/main.rs:521

  • Dispatch-time validation rejections never produce the promised structured mxc.ConfigRejected record. For example, an invalid IsolationSession user or a forbidden later-phase filesystem field returns PolicyValidation from run_state_aware; this path only emits the prose diagnostic at line 568. Emit ConfigRejected for pre-execution validation/unsupported errors here (while excluding genuine runtime backend failures), with reason mappings that reflect the actual validation cause.
    let mut outcome = mxc_engine::run_state_aware(parsed, dry_run);

src/backends/appcontainer/common/src/network_manager.rs:475

  • This records an attempted stop as a successful stop. ProxyCoordinator::stop returns no outcome, while its cleanup helper can fail to signal a process or time out with the child still alive; this code nevertheless sets proxy_stopped=true, causing mxc.SandboxTornDown to claim cleanup succeeded. Return a cleanup result from the coordinator and set this field only when the proxy processes and policy were actually released.
        if self.proxy_coordinator.is_active() {
            self.proxy_coordinator.stop(logger);
            outcome.proxy_stopped = true;

src/core/wxc_common/src/policy_identity.rs:215

  • Blanket serialization makes the hash sensitive to non-enforcement fields and bypasses the stated allow-list tripwire. WindowsSandboxConfig currently contains idle_timeout_ms and daemon_pipe_name, both documented as legacy values ignored by the one-shot backend, so changing either changes mxc.PolicyHash even though the effective policy is identical. Explicitly project/destructure only enforcement-relevant nested fields instead of serializing the whole struct.
    out.insert(
        "windows_sandbox".into(),
        serde_json::to_value(windows_sandbox).unwrap_or(Value::Null),

src/core/wxc/src/main.rs:1164

  • ParseError::Decode is not limited to malformed JSON: the initial RequestDiscriminator deserialize also returns it for valid JSON of the wrong shape (for example, a string or array root). Those requests are therefore recorded as malformed_json instead of schema_violation. Preserve ConfigDeserializeError::classify() through the discriminator path and map data-category failures to the schema reason.
            log_config_rejected(
                &mut logger,
                RejectionReason::MalformedJson,
                UNKNOWN_BACKEND,

docs/telemetry/telemetry.md:452

  • This paragraph is now false: the new thread-local diagnostic sink is inherited in state-aware IsolationSession exec, and that inherited logger is passed to create_process, whose manager emits ProcessExited, ProcessTimedOut, and ProcessKillFailed. Update this paragraph and the platform/requirements tables so they describe state-aware IsolationSession process-event coverage accurately.
The isolation-session `ProcessTimedOut` and `ProcessKillFailed` records are
emitted by the one-shot runner, where `wxc-exec` supplies the local diagnostic
logger to the backend. The state-aware backend trait does not currently carry a
logger into its `exec` method, so state-aware isolation-session exec remains
unrecorded by these MXC-local process-boundary events.

Comment thread src/core/wxc/src/main.rs
let incoming_sandbox_id = parsed.sandbox_id.clone();
// State-aware dispatch bypasses the one-shot runner funnel, so anchor the
// effective lifecycle policy here before the request is consumed.
mxc_engine::log_policy_hash(&parsed.request, logger);
) -> Result<bool, WxcError> {
) -> Result<(), WxcError> {
let outcome = self.apply_firewall_rules_inner(principal_id, policy, logger);
self.firewall_apply_ok = Some(outcome.is_ok());
Comment on lines +162 to +168
// Never persist Entra credentials or account identifiers in diagnostics.
if let Some(isolation_session) = redacted.experimental.isolation_session.as_mut() {
if let Some(user) = isolation_session.user.as_mut() {
user.upn = "<redacted>".to_string();
user.wam_token = "<redacted>".to_string();
}
}
Co-authored-by: RamonArjona4 <25335379+RamonArjona4@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 8, 2026 01:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.

Suppressed comments (10)

src/core/wxc_common/src/audit.rs:423

  • This sanitizer cannot know whether a matching shape was MXC-minted: AppContainer passes the caller-controlled containerId here, so a caller can choose sandbox-0123456789abcdef (or CLI) and have that config value logged verbatim. This contradicts the stated rule that caller-supplied identities are always redacted. Preserve provenance at the call sites (for example, separate APIs/newtypes for minted state-aware IDs versus config-derived names) instead of inferring it from the string shape.
    if let Some(hex) = identity.strip_prefix("sandbox-") {
        if hex.len() == SANDBOX_ID_HEX_LEN
            && hex
                .bytes()
                .all(|b| b.is_ascii_digit() || (b'a'..=b'f').contains(&b))
        {
            return identity;
        }
        return REDACTED_IDENTITY;

src/backends/appcontainer/common/src/network_manager.rs:286

  • process_host_list still converts every per-host create_rule failure into continue and returns Ok(()). Consequently this stores Some(true) and the new audit record reports status=success even when a blocked-host rule failed to install (leaving enforcement weaker than requested). Propagate/aggregate those failures before setting firewall_apply_ok.
        let outcome = self.apply_firewall_rules_inner(principal_id, policy, logger);
        self.firewall_apply_ok = Some(outcome.is_ok());
        outcome

src/backends/appcontainer/common/src/appcontainer_runner.rs:1826

  • The Drop path also tears down firewall/BFS policy after only five seconds even if kill() failed and the process remains active. A dropped handle can therefore leave live sandboxed code running after its enforcement is removed. Retain the previous unbounded reap, or explicitly skip teardown while the process is still active.
        let _ = self.kill();
        unsafe {
            let _ = WaitForSingleObject(self.process.get(), REAP_AFTER_KILL_TIMEOUT_MS);

src/backends/appcontainer/common/src/appcontainer_runner.rs:1750

  • This propagates TerminateJobObject failures through SandboxProcess::kill, changing the previous best-effort API behavior and contradicting both UiJobObject::terminate's new contract and the added documentation that kill failures are recorded but never propagated. Log the failure and keep returning Ok(()), unless this breaking behavior change is intentional and documented.
                return Err(std::io::Error::other(format!(
                    "TerminateJobObject failed: {error}"
                )));

src/backends/isolation_session/common/src/manager.rs:690

  • The successful retry receives only a one-second reap window, while the initial successful Terminate() gets the documented five seconds. A retry that lands but needs 1–5 seconds is incorrectly reported as still active. Use the same five-second post-termination wait here.
            let _ = process.WaitForExit(1000);
            exit_code = process.ExitCode().unwrap_or(STILL_ACTIVE);

src/core/wxc/src/main.rs:512

  • For post-provision phases, parsed.request does not contain the sandbox's immutable provision-time policy; the parser leaves omitted policy fields at defaults and carries only exec process data (which the hash deliberately excludes). Thus exec/start/stop records for differently provisioned sandboxes can emit the same PolicyHash, falsely presenting it as the effective policy. Persist/relay the provision-time hash for later phases, or emit this record only where the effective policy is actually available.
    // State-aware dispatch bypasses the one-shot runner funnel, so anchor the
    // effective lifecycle policy here before the request is consumed.
    mxc_engine::log_policy_hash(&parsed.request, logger);

src/backends/appcontainer/common/src/network_manager.rs:475

  • stop() is best-effort: signal_process_cleanup can fail SetEvent or time out after five seconds, but this unconditionally records proxy_stopped=true whenever the coordinator was active. That makes SandboxTornDown claim a release that may not have happened. Return the observed stop outcome from ProxyCoordinator::stop and use it here.
        if self.proxy_coordinator.is_active() {
            self.proxy_coordinator.stop(logger);
            outcome.proxy_stopped = true;

src/backends/appcontainer/common/src/base_container_runner.rs:2339

  • This has the same false-success issue as AppContainer teardown: proxy_stopped is captured before calling a best-effort stop() whose child cleanup may time out or fail. The audit record then reports the proxy as stopped regardless. Record the actual stop result once ProxyCoordinator::stop exposes it.
        let proxy_stopped = self.proxy_coordinator.is_active();
        self.proxy_coordinator.stop(&mut logger);

src/core/wxc/src/main.rs:1163

  • ParseError::Decode also represents a missing/unreadable config file and valid JSON with the wrong top-level shape, not only malformed JSON. Those cases now produce a machine-readable reason=malformed_json, which is objectively false. Split input acquisition, JSON syntax, and schema/data failures into distinct typed variants/reasons rather than collapsing every Decode error here.
        Err(ParseError::Decode(_)) => {
            // The payload could not even be decoded into JSON, so no backend or
            // field path is known — the record still exists so a rejected run
            // is never invisible.
            log_config_rejected(
                &mut logger,
                RejectionReason::MalformedJson,

docs/telemetry/telemetry.md:452

  • This is outdated by the code in this PR: state-aware isolation-session exec now inherits the driver's sinks and passes that logger into create_process, so it does emit these process-boundary records. Update this paragraph, the platform-scope row above, and the M-ETW-1 row below to describe the implemented state-aware coverage.
The isolation-session `ProcessTimedOut` and `ProcessKillFailed` records are
emitted by the one-shot runner, where `wxc-exec` supplies the local diagnostic
logger to the backend. The state-aware backend trait does not currently carry a
logger into its `exec` method, so state-aware isolation-session exec remains
unrecorded by these MXC-local process-boundary events.

Comment on lines +1808 to +1810
let _ = self.kill();
unsafe {
let _ = WaitForSingleObject(self.process.get(), u32::MAX);
let _ = WaitForSingleObject(self.process.get(), REAP_AFTER_KILL_TIMEOUT_MS);
Co-authored-by: RamonArjona4 <25335379+RamonArjona4@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 8, 2026 01:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.

Suppressed comments (9)

src/core/wxc/src/main.rs:512

  • parsed.request is not the effective state-aware policy. For non-provision phases its containment is typically the default ProcessContainer (the real backend is resolved from sandboxId), and phase-specific enforcement such as experimental.isolation_session.start.configurationId remains only in experimental_raw. This therefore records the wrong backend and gives identical hashes to materially different state-aware policies. Build the identity from the resolved backend plus the redacted, typed phase config before dispatch.
    mxc_engine::log_policy_hash(&parsed.request, logger);

src/core/wxc_common/src/diagnostic.rs:215

  • This redactor still emits proxy credentials. network.proxy.url is keyed as url, so redact_secret_fields leaves a value such as http://user:password@host:8080 unchanged and the newly added raw-config diagnostic writes it verbatim. Redact URL userinfo on the known proxy URL path (the existing proxy_env::redact_proxy_url helper can be reused) before serialization.
pub fn redact_raw_config_json(raw: &str) -> String {
    match serde_json::from_str::<serde_json::Value>(raw) {
        Ok(mut value) => {
            redact_secret_fields(&mut value);
            serde_json::to_string_pretty(&value)
                .unwrap_or_else(|_| "<unable to re-serialize redacted config>".to_string())
        }
        Err(_) => "<unparsable JSON config, omitted from diagnostics>".to_string(),

src/backends/appcontainer/common/src/base_container_runner.rs:2401

  • A genuine termination failure is logged but then converted to Ok(()). terminate_and_reap immediately ignores that result and waits with u32::MAX, so a timed-out or abandoned process can hang the executor forever when TerminateJobObject/TerminateProcess fails. Propagate the failure to kill() and use a bounded reap/recheck before teardown.
        if let Err((method, error)) = outcome {
            if !self.audit_enabled() {
                return Ok(());
            }
            let record = self

src/backends/appcontainer/common/src/appcontainer_runner.rs:1734

  • The current callers still ignore this propagated error and then perform WaitForSingleObject(..., u32::MAX) in both wait() and Drop. Thus a real TerminateJobObject failure (not the already-exited race) still hangs indefinitely instead of returning the failure. Make the normal wait path honor the kill error and bound/recheck the reap wait; the Drop path also needs a bounded fallback.
        if let Err(error) = self.job.terminate(u32::MAX) {
            // `TerminateJobObject` fails with `ERROR_ACCESS_DENIED` once every
            // process in the job has already exited -- that's a benign race,
            // not a genuine termination failure. Recheck before deciding
            // whether to report a clean teardown or propagate the failure.
            let already_exited = matches!(self.try_wait(), Ok(Some(_)));
            if !already_exited {

src/core/wxc_common/src/policy_identity.rs:216

  • Blanket-serializing windows_sandbox makes ignored compatibility settings part of the policy identity. WindowsSandboxConfig::idle_timeout_ms and daemon_pipe_name are explicitly documented as ignored by the one-shot backend (models.rs:239-244), yet changing either changes this hash, violating the stated invariant that non-enforcement settings do not affect identity. Project only enforcement-relevant fields and add a regression test for ignored fields.
    out.insert(
        "windows_sandbox".into(),
        serde_json::to_value(windows_sandbox).unwrap_or(Value::Null),
    );

docs/telemetry/telemetry.md:452

  • This is now outdated: state-aware IsolationSession exec inherits the driver's diagnostic sink and passes that logger into create_process, so it emits ProcessExited, ProcessTimedOut, and ProcessKillFailed. Update this paragraph and the coverage entries at lines 438 and 458; otherwise operators are told to expect these records to be absent when the implementation emits them.
The isolation-session `ProcessTimedOut` and `ProcessKillFailed` records are
emitted by the one-shot runner, where `wxc-exec` supplies the local diagnostic
logger to the backend. The state-aware backend trait does not currently carry a
logger into its `exec` method, so state-aware isolation-session exec remains
unrecorded by these MXC-local process-boundary events.

src/core/wxc_common/src/audit.rs:70

  • This documentation says SandboxIdentity is emitted once per lifecycle, but run_state_aware_main emits it after every successful phase. Describe the actual per-phase behavior so consumers do not incorrectly deduplicate or expect a single record.
    /// The sandbox identity join key, emitted once per lifecycle.
    SandboxIdentity,

src/core/wxc_common/src/audit.rs:53

  • This record is also emitted for IsolationSession processes, so describing it as ProcessContainer-only is incorrect.
    /// A `process_container` sandboxed process exited on its own.
    ProcessExited,

src/core/wxc_common/src/audit.rs:45

  • Remove the stray documentation delimiter embedded in this sentence.
/// Closed set of audit record names. The `mxc.` prefix namespaces the record/// against unrelated lines sharing the same sink.

Comment on lines +162 to +166
// Never persist Entra credentials or account identifiers in diagnostics.
if let Some(isolation_session) = redacted.experimental.isolation_session.as_mut() {
if let Some(user) = isolation_session.user.as_mut() {
user.upn = "<redacted>".to_string();
user.wam_token = "<redacted>".to_string();
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants